Add support for local variables in find-references#1231
Conversation
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Did some basic testing and it seems to work
| pub fn child_scopes(&self, scope: ScopeId) -> ChildScopesIter<'_> { | ||
| let descendants = &self.scopes[scope].descendants; | ||
| ChildScopesIter { | ||
| pub fn child_scope_ids(&self, scope_id: ScopeId) -> ChildScopeIdsIter<'_> { |
There was a problem hiding this comment.
i approve of this naming!
| pub fn scope_ids(&self) -> impl Iterator<Item = ScopeId> + '_ { | ||
| self.scopes.iter().map(|(id, _)| id) | ||
| } |
There was a problem hiding this comment.
Do you think it would be more useful and general to just expose this?
pub fn scopes(&self) -> &IndexVec<ScopeId, Scope>Or do you purposefully try not to directly expose the IndexVec?
There was a problem hiding this comment.
hmm no we do expose IndexVec elsewhere, so i do feel like this might be more useful to expose?
pub fn uses(&self, scope: ScopeId) -> &IndexVec<UseId, Use> {
| scope: ScopeId, | ||
| ) -> Option<(ScopeId, DefinitionId, &Definition)> { | ||
| for ancestor in self.ancestor_scopes(scope) { | ||
| for ancestor in self.ancestor_scope_ids(scope) { |
There was a problem hiding this comment.
| scope: ScopeId, | |
| ) -> Option<(ScopeId, DefinitionId, &Definition)> { | |
| for ancestor in self.ancestor_scopes(scope) { | |
| for ancestor in self.ancestor_scope_ids(scope) { | |
| scope_id: ScopeId, | |
| ) -> Option<(ScopeId, DefinitionId, &Definition)> { | |
| for ancestor_id in self.ancestor_scope_ids(scope) { |
probably would be useful to do a Claude pass once all of this is merged that does a mass renaming for consistency
There was a problem hiding this comment.
I'll add it to the salsa post-pr bullet list
There was a problem hiding this comment.
the changes in identifier.rs make this feeeeel more smooth!
|
|
||
| // Check and see if we got an identifier. If we didn't, we might need to use | ||
| // some heuristics to look around. Unfortunately, it seems like if you double-click | ||
| // to select an identifier, and then use Right Click -> Find All References, the | ||
| // position received by the LSP maps to the _end_ of the selected range, which | ||
| // is technically not part of the associated identifier's range. In addition, we | ||
| // can't just subtract 1 from the position column since that would then fail to | ||
| // resolve the correct identifier when the cursor is located at the start of the | ||
| // identifier. | ||
| // Zero-width range queries at an identifier boundary return the | ||
| // wrapping node rather than the identifier itself. If the cursor is at | ||
| // the trailing edge of a selection (column past the last character), | ||
| // retry one column back. If it's at the leading edge (column on the | ||
| // first character), retry one column forward. | ||
| if !node.is_identifier() && point.column > 0 { | ||
| let point = Point::new(point.row, point.column - 1); | ||
| node = ast | ||
| let back = Point::new(point.row, point.column - 1); | ||
| if let Some(retry) = ast | ||
| .root_node() | ||
| .descendant_for_point_range(point, point) | ||
| .into_result()?; | ||
| .descendant_for_point_range(back, back) | ||
| .filter(|n| n.is_identifier()) | ||
| { | ||
| node = retry; | ||
| } | ||
| } | ||
| if !node.is_identifier() { | ||
| let fwd = Point::new(point.row, point.column + 1); | ||
| if let Some(retry) = ast |
There was a problem hiding this comment.
i dont remember the details right now but id be pretty hesitant to change this right now
im not sure we really have a big suite of tests for this, but it seems like this was from some hard fought debugging based on the comment
can we just leave it as is till we remove it for good?
There was a problem hiding this comment.
I'm just expanding the workaround, with a test. I've added a test for the original workaround too. It doesn't seem like a very risky path, so I think I'd prefer to go ahead with the change. Otherwise we'll need to (a) keep a hole in the impl, and (b) keep a hole in the test coverage, both with fixmes.
| // Some LSP clients send the cursor one past the last character of a | ||
| // selected identifier (typical for double-click then "find references"). | ||
| // We exercise this through the cross-file fallback because `mutate` is | ||
| // unbound: the intra-file pass returns nothing, and `build_context`'s | ||
| // boundary retry pulls the cursor back one column onto the identifier | ||
| // before the textual walk runs. | ||
| let dir = tempfile::tempdir().unwrap(); |
There was a problem hiding this comment.
can we also exercise this in the intra file case?
it is an important use case, because its probably how i do find references 99% of the time (double click first, which puts my flashing cursor at the end of mutate, then right click and "find references")
There was a problem hiding this comment.
Added a test, and a fix because the test was failing.
We now have a rudimentary version of r-a's pick_best_token() that snaps to a neighbouring identifier token when the offset sits in between tokens.
| // Walk all uses in every scope and check for each use of the same name | ||
| // whether its binding set intersects the target. | ||
| for scope_id in index.scope_ids() { | ||
| let symbols = index.symbols(scope_id); | ||
| let Some(symbol_id) = symbols.id(&name) else { | ||
| // The scope doesn't have any uses for that symbol | ||
| continue; | ||
| }; | ||
|
|
||
| for (use_id, use_site) in index.uses(scope_id).iter() { | ||
| if use_site.symbol() != symbol_id { | ||
| continue; | ||
| } | ||
| let intersects = index | ||
| .reaching_definitions(scope_id, use_id) | ||
| .any(|d| target_defs.contains(&d)); | ||
| if !intersects { | ||
| continue; | ||
| } | ||
| results.push(FileRange { | ||
| file: pos.file.clone(), | ||
| range: use_site.range(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Whew, find_references() is complicated! So IIUC, with:
foo <- function() {}
foo() # <- find references from here
bar()
foo()We first classify() foo to figure out that it is a Identifier::Use and collect its reaching_definitions() of foo <- function() {} as target_defs
But that's really only the first part of find_references(). The real part is figuring out where else this thing is used.
And I guess we don't directly store that info in the SemanticIndex on an individual Definition, so we have to hunt for it.
So we scan the file top to bottom looking for uses of the foo symbol, and we see if its set of reaching definitions match ours, and those become part of the final result.
Now it totally makes sense why other groups do a textual scan as part of this feature.
Scanning through every single file in the workspace from top to bottom would nuke performance. So the textual scan can probably at least do the symbol matching loop for you across all files very quickly, and then we'd just be in charge of matching up the uses of all of the hits with our target_defs!
There was a problem hiding this comment.
Without the textual search I guess we'd maintain a map of some sort.
lionel-
force-pushed
the
oak/goto-def-intra-file
branch
from
7e6ad7c to
6fa14ce
Compare
lionel-
force-pushed
the
oak/find-refs-intra-file
branch
from
e150326 to
2bc2610
Compare
lionel-
force-pushed
the
oak/find-refs-intra-file
branch
from
82d3822 to
014546a
Compare
2 participants
Branched from #1230
Part of posit-dev/positron#13749
Part of #1149
Adds a within-file Oak path to the existing find-references handler of the Ark LSP.
Same deal as for goto-definition: within-file behaviour is based on the semantic index and precise, cross-file is handled by the legacy handler and is approximate (matches any symbol of the same name without filtering based on expected definition).